Skip to content

backport: Merge bitcoin/bitcoin#28857 , 29458, 29236, 29459, 29479, 29615, 29433#7292

Open
vijaydasmp wants to merge 7 commits into
dashpay:developfrom
vijaydasmp:May_2026_02
Open

backport: Merge bitcoin/bitcoin#28857 , 29458, 29236, 29459, 29479, 29615, 29433#7292
vijaydasmp wants to merge 7 commits into
dashpay:developfrom
vijaydasmp:May_2026_02

Conversation

@vijaydasmp
Copy link
Copy Markdown

bitcoin backports

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 26, 2026

fanquake and others added 3 commits April 29, 2026 20:11
1e5b861 test: Add test for array serialization (TheCharlatan)
d49d198 refactor: Initialize magic bytes in constructor initializer (TheCharlatan)

Pull request description:

  This is a followup-PR for bitcoin#28423

  * Initialize magic bytes in constructor
  * Add a small unit test for serializing arrays.

ACKs for top commit:
  sipa:
    utACK 1e5b861
  maflcko:
    lgtm ACK 1e5b861

Tree-SHA512: 0f58d2332dc501ca9fd419f40ed4f977c83dce0169e9a0eee1ffc9f8daa2d2ef7e7df18205ba076f55d90ae6c4a20d2b51ab303150d38470a962bcc58a66f6e7
…void resizing

a19235c Preallocate result in `TryParseHex` to avoid resizing (Lőrinc)
b7489ec Add benchmark for TryParseHex (Lőrinc)

Pull request description:

  This pull request introduces optimizations to the `TryParseHex` function, focusing primarily on the ideal case (valid hexadecimal input without spaces).
  A new benchmark, `HexParse` was introduced in a separate commit.

  The main optimization preallocates the result vector based on the input string's length. This aims to completely avoid costly dynamic reallocations when no spaces are present.

  ------------

  Before:
  ```
  |           ns/base16 |            base16/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |                1.60 |      623,238,893.11 |    0.3% |      0.01 | `HexParse`
  |                1.65 |      606,747,566.34 |    0.6% |      0.01 | `HexParse`
  |                1.60 |      626,149,544.07 |    0.3% |      0.01 | `HexParse`
  ```

  After:
  ```
  |           ns/base16 |            base16/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |                0.68 |    1,465,555,976.27 |    0.8% |      0.01 | `HexParse`
  |                0.68 |    1,472,962,920.18 |    0.3% |      0.01 | `HexParse`
  |                0.68 |    1,476,159,423.00 |    0.3% |      0.01 | `HexParse`
  ```

ACKs for top commit:
  achow101:
    ACK a19235c
  hebasto:
    ACK a19235c.
  andrewtoth:
    Re-ACK a19235c
  Empact:
    Re-ACK bitcoin@a19235c

Tree-SHA512: e09a59791104be3fd1026862ce98de9efafa1f949626fa01e3b7d58e6a2ef02a11f0de55ddba5c43230a53effd24e6d368c1e12848b17e8ce91d7908a59333f0
fa39151 refactor: Remove unused error() (MarcoFalke)
fad0335 scripted-diff: Replace error() with LogError() (MarcoFalke)
fa808fb refactor: Make error() return type void (MarcoFalke)
fa1d624 scripted-diff:  return error(...);  ==>  error(...); return false; (MarcoFalke)
fa9a5e8 refactor: Add missing {} around error() calls (MarcoFalke)

Pull request description:

  `error(...)` has many issues:

  * It is often used in the context of `return error(...)`, implying that it has a "fancy" type, creating confusion with `util::Result/Error`
  * `-logsourcelocations` does not work with it, because it will pretend the error happened inside of `logging.h`
  * The log line contains `ERROR: `, as opposed to `[error]`, like for other errors logged with `LogError`.

  Fix all issues by removing it.

ACKs for top commit:
  fjahr:
    re-utACK fa39151
  stickies-v:
    re-ACK fa39151, no changes since 4a90374
  ryanofsky:
    Code review ACK fa39151. Just rebase since last review

Tree-SHA512: ec5bb502ab0d3733fdb14a8a00762638fce0417afd8dd6294ae0d485ce2b7ca5b1efeb50fc2cd7467f6c652e4ed3e99b0f283b08aeca04bbfb7ea4f2c95d283a

Rebase and fixing blockstorage.cpp file

src/validation.cpp - Fixing linting error
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin/bitcoin#28857 backport: Merge bitcoin/bitcoin#28857 , 29458, 29236, 29633, 29459, 29479, 29615, 29433 Apr 29, 2026
@vijaydasmp vijaydasmp marked this pull request as ready for review April 30, 2026 08:02
@thepastaclaw
Copy link
Copy Markdown

thepastaclaw commented Apr 30, 2026

✅ Review complete (commit 16c7f15)

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

Review Change Stack

Walkthrough

This PR introduces three independent improvements: (1) a --json flag for rpcauth.py to output credentials as JSON, (2) fixes to script opcode handling and multisig signature operation counting in the test framework with corresponding test coverage, and (3) consolidation of lint-directory exclusions into a shared configuration module. The changes span the RPC auth utility, test framework, and lint infrastructure, with no modifications to core consensus or validation logic in this subset of ranges.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • thepastaclaw
  • UdjinM6
  • PastaPastaPasta
🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.48% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title lists multiple bitcoin backport PR numbers but does not clearly summarize the main change in the changeset. Consider revising to emphasize the primary objective (e.g., 'Remove legacy error() logging helper and backport related changes') or breaking into focused commits per backport.
Description check ❓ Inconclusive The description 'bitcoin backports' is very generic and vague, using a non-descriptive term that conveys minimal meaningful information about the changeset. Expand the description to outline key changes such as logging refactoring, hex parsing optimization, rpcauth JSON support, and lint centralization.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
test/lint/lint-includes.py (1)

20-26: ⚡ Quick win

Deduplicate EXCLUDED_DIRS now that shared exclusions are imported.

Line 20-23 duplicates paths already provided by SHARED_EXCLUDED_SUBTREES, leaving two sources of truth.

Proposed refactor
-EXCLUDED_DIRS = ["src/leveldb/",
-                 "src/crc32c/",
-                 "src/secp256k1/",
-                 "src/minisketch/",
-                 "src/dashbls/",
-                 "src/immer/",
-                 "src/crypto/x11/"] + SHARED_EXCLUDED_SUBTREES
+EXCLUDED_DIRS = [
+    "src/dashbls/",
+    "src/immer/",
+    "src/crypto/x11/",
+] + SHARED_EXCLUDED_SUBTREES
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/lint/lint-includes.py` around lines 20 - 26, The EXCLUDED_DIRS list
duplicates paths that are already present in SHARED_EXCLUDED_SUBTREES; update
the definition of EXCLUDED_DIRS to avoid redundant entries by removing the
hardcoded directory strings and simply compose EXCLUDED_DIRS from any unique
local exclusions (if any) plus SHARED_EXCLUDED_SUBTREES, ensuring EXCLUDED_DIRS
references SHARED_EXCLUDED_SUBTREES (symbol names: EXCLUDED_DIRS,
SHARED_EXCLUDED_SUBTREES) so there is a single source of truth for shared
exclusions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/node/blockstorage.cpp`:
- Around line 807-810: In ReadBlockFromDisk (the branch that currently does
`return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match
index for %s at %s", pindex->ToString(), block_pos.ToString()); return false;`)
remove the unreachable `return false;` and instead replace the `return
error(...)` call with a logging call (e.g., LogError or the project's LogPrint
equivalent) that emits the same message using pindex->ToString() and
block_pos.ToString(), then follow it with `return false;` so the branch matches
the LogError(...); return false; pattern used elsewhere.

In `@src/validation.cpp`:
- Around line 2484-2488: The LogError call after Consensus::CheckTxInputs is
logging state.ToString() which is incorrect because the CheckTxInputs failure
details are in tx_state; update the LogError invocation in the ConnectBlock
error branch to include tx_state.ToString() (and/or
tx_state.GetRejectReason()/GetDebugMessage() if you prefer more detail) so the
actual transaction rejection reason from CheckTxInputs is recorded before
returning state.Invalid; keep the existing return that uses tx_state for the
reject reason/debug message.

In `@test/lint/lint-spelling.py`:
- Line 18: The list comprehension uses the loop variable named dir which shadows
the built-in dir() and triggers Ruff A001; update the variable name in the
expression that builds FILES_ARGS (the comprehension referencing
SHARED_EXCLUDED_SUBTREES) to a non-built-in name like subtree or subdir so it no
longer shadows the built-in (e.g., replace occurrences of dir inside the
comprehension with subtree).

---

Nitpick comments:
In `@test/lint/lint-includes.py`:
- Around line 20-26: The EXCLUDED_DIRS list duplicates paths that are already
present in SHARED_EXCLUDED_SUBTREES; update the definition of EXCLUDED_DIRS to
avoid redundant entries by removing the hardcoded directory strings and simply
compose EXCLUDED_DIRS from any unique local exclusions (if any) plus
SHARED_EXCLUDED_SUBTREES, ensuring EXCLUDED_DIRS references
SHARED_EXCLUDED_SUBTREES (symbol names: EXCLUDED_DIRS, SHARED_EXCLUDED_SUBTREES)
so there is a single source of truth for shared exclusions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4c0b72be-d976-4838-b731-76d2d55c7508

📥 Commits

Reviewing files that changed from the base of the PR and between 7e524b1 and 11dde62.

📒 Files selected for processing (26)
  • share/rpcauth/README.md
  • share/rpcauth/rpcauth.py
  • src/Makefile.bench.include
  • src/addrdb.cpp
  • src/bench/parse_hex.cpp
  • src/flatfile.cpp
  • src/index/base.cpp
  • src/index/blockfilterindex.cpp
  • src/index/coinstatsindex.cpp
  • src/index/txindex.cpp
  • src/kernel/coinstats.cpp
  • src/logging.h
  • src/net.cpp
  • src/netbase.cpp
  • src/node/blockstorage.cpp
  • src/script/signingprovider.cpp
  • src/test/serialize_tests.cpp
  • src/util/strencodings.cpp
  • src/validation.cpp
  • test/functional/mempool_accept.py
  • test/functional/test_framework/script.py
  • test/lint/README.md
  • test/lint/lint-include-guards.py
  • test/lint/lint-includes.py
  • test/lint/lint-spelling.py
  • test/lint/lint_ignore_dirs.py

Comment thread src/node/blockstorage.cpp
Comment on lines 807 to 810
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove unreachable return and complete the error() cleanup in this branch.

At Line 807, return error(...); makes Line 809 unreachable. Convert this branch to the same LogError(...); return false; pattern used in this PR.

Suggested fix
-    if (*hash != pindex->GetBlockHash()) {
-        return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
-                     pindex->ToString(), block_pos.ToString());
-        return false;
-    }
+    if (*hash != pindex->GetBlockHash()) {
+        LogError("%s: GetHash() doesn't match index for %s at %s\n",
+                 __func__, pindex->ToString(), block_pos.ToString());
+        return false;
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/node/blockstorage.cpp` around lines 807 - 810, In ReadBlockFromDisk (the
branch that currently does `return error("ReadBlockFromDisk(CBlock&,
CBlockIndex*): GetHash() doesn't match index for %s at %s", pindex->ToString(),
block_pos.ToString()); return false;`) remove the unreachable `return false;`
and instead replace the `return error(...)` call with a logging call (e.g.,
LogError or the project's LogPrint equivalent) that emits the same message using
pindex->ToString() and block_pos.ToString(), then follow it with `return false;`
so the branch matches the LogError(...); return false; pattern used elsewhere.

Comment thread src/validation.cpp
Comment on lines 2484 to 2488
if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
// Any transaction validation failure in ConnectBlock is a block consensus failure
LogPrintf("ERROR: %s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString());
LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString());
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
tx_state.GetRejectReason(), tx_state.GetDebugMessage());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Log the failing tx_state instead of state.

Line 2486 logs state.ToString(), but this branch hasn't copied the CheckTxInputs() failure into state yet. That can hide the actual rejection reason in debug.log and make consensus failures harder to diagnose.

Suggested fix
             if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
                 // Any transaction validation failure in ConnectBlock is a block consensus failure
-                LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString());
+                LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), tx_state.ToString());
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
                             tx_state.GetRejectReason(), tx_state.GetDebugMessage());
             }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
// Any transaction validation failure in ConnectBlock is a block consensus failure
LogPrintf("ERROR: %s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString());
LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString());
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
tx_state.GetRejectReason(), tx_state.GetDebugMessage());
if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
// Any transaction validation failure in ConnectBlock is a block consensus failure
LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), tx_state.ToString());
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
tx_state.GetRejectReason(), tx_state.GetDebugMessage());
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/validation.cpp` around lines 2484 - 2488, The LogError call after
Consensus::CheckTxInputs is logging state.ToString() which is incorrect because
the CheckTxInputs failure details are in tx_state; update the LogError
invocation in the ConnectBlock error branch to include tx_state.ToString()
(and/or tx_state.GetRejectReason()/GetDebugMessage() if you prefer more detail)
so the actual transaction rejection reason from CheckTxInputs is recorded before
returning state.Invalid; keep the existing return that uses tx_state for the
reject reason/debug message.

IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt'
FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crc32c/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/leveldb/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/util/subprocess.hpp", ":(exclude)src/wallet/bip39_english.h"]
FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/dashbls/", ":(exclude)src/crypto/", ":(exclude)src/ctpl_stl.h", ":(exclude)src/cxxtimer.hpp", ":(exclude)src/immer/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)contrib/builder-keys/", ":(exclude)contrib/guix/patches", ":(exclude)src/util/subprocess.hpp", ":(exclude)src/wallet/bip39_english.h"]
FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n test/lint/lint-spelling.py | head -25

Repository: dashpay/dash

Length of output: 1481


🏁 Script executed:

find . -maxdepth 2 -type f \( -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" -o -name "setup.cfg" \) | head -10

Repository: dashpay/dash

Length of output: 68


🏁 Script executed:

cat ./ruff.toml

Repository: dashpay/dash

Length of output: 1171


Rename loop variable dir to avoid shadowing Python built-in.

Line 18 uses dir as a loop variable, which shadows the built-in dir() function and triggers Ruff A001. This will fail lint CI since the project's ruff.toml enables the A rule (flake8-builtins) without suppressing this check.

Proposed fix
-FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES]
+FILES_ARGS += [f":(exclude){excluded_dir}" for excluded_dir in SHARED_EXCLUDED_SUBTREES]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES]
FILES_ARGS += [f":(exclude){excluded_dir}" for excluded_dir in SHARED_EXCLUDED_SUBTREES]
🧰 Tools
🪛 Ruff (0.15.12)

[error] 18-18: Variable dir is shadowing a Python builtin

(A001)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/lint/lint-spelling.py` at line 18, The list comprehension uses the loop
variable named dir which shadows the built-in dir() and triggers Ruff A001;
update the variable name in the expression that builds FILES_ARGS (the
comprehension referencing SHARED_EXCLUDED_SUBTREES) to a non-built-in name like
subtree or subdir so it no longer shadows the built-in (e.g., replace
occurrences of dir inside the comprehension with subtree).

Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Validated all findings against source. One blocking merge-resolution defect in blockstorage.cpp (stale return error(...) plus unreachable return false) and five LogError calls in validation.cpp that lost their trailing \n during the bitcoin#29236 conversion. Two informational backport-prerequisite notes (partial adaptations of bitcoin#28857 and bitcoin#29236) confirmed against current Dash code.

Reviewed commit: 11dde62

🔴 1 blocking | 🟡 5 suggestion(s) | 💬 2 nitpick(s)

1 additional finding

💬 nitpick: bitcoin#29236 only partially adopted: Dash retains its own error() template

src/util/system.h (lines 52-57)

Verified: bitcoin#29236 upstream removed the error(...) template entirely. Dash applied the scripted-diff converting many error(...) callers to LogError(...) + return false, but kept the error() template at src/util/system.h:53. Several Dash-specific call sites still rely on it (e.g., src/node/blockstorage.cpp:776,784,791,807; src/util/system.cpp:120; src/validation.cpp:2039; src/evo/specialtxman.cpp:593). The retained template wraps LogPrintf rather than LogError, so log severity for those sites is Info rather than Error. Additionally, bitcoin#29633 updated logging.h's leading comment to drop the explicit error reference; this leaves the comment slightly out of sync with Dash's actual state where error() still exists. Cosmetic / informational only — flag for tracking the eventual full removal.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/node/blockstorage.cpp`:
- [BLOCKING] lines 806-810: Stale return error(...) with unreachable return false; bitcoin#29236 conversion missed this site
  Confirmed at src/node/blockstorage.cpp:806-810:

```cpp
if (*hash != pindex->GetBlockHash()) {
    return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
                 pindex->ToString(), block_pos.ToString());
    return false;
}

Two problems: (1) error(...) already returns false, so the trailing return false; is unreachable — this can trip the configured -Wunreachable-code diagnostics when warnings are promoted to errors. (2) The point of the bitcoin#29236 backport in this PR is precisely to nuke error() from these call sites; every other hunk in the diff converts to LogError(...\n); return false;. This single site was missed during merge resolution.

Functionally the function still returns false on hash mismatch, so consensus is unaffected, but the merge resolution is inconsistent and risks breaking warning-as-error builds.

In src/validation.cpp:

  • [SUGGESTION] line 4781: LogError missing trailing \n in RollforwardBlock
    Confirmed: line ends with ...ToString()); and no \n in the format string. LogError expands to LogPrintLevel_LogPrintf_tfm::format(fmt, args...) and writes the result verbatim via LogPrintStr. The Logger uses the trailing \n to drive m_started_new_line (logging.cpp:481), which controls whether the next entry gets its timestamp/source-location/threadname prefix. Without a trailing \n the next log line gets concatenated onto the same line and loses its prefix. Every other LogError introduced by this PR (e.g., lines 4690, 4755, 4897, 4909, 4916) ends in \n; this one was missed.
  • [SUGGESTION] line 4924: LogError missing trailing \n on Dash-specific EvoDB inconsistency message
    Same root cause as the line 4781 finding: this Dash-specific call (note the (DASH): tag) was converted from error(...) but lost the newline. The very next LogError call at line 4936 (also Dash-specific) has the same defect, as do 4942 and 5126.
  • [SUGGESTION] line 4936: LogError missing trailing \n in ReplayBlocks ReadBlockFromDisk failure
    Same missing-newline issue as 4781/4924. Format string ends with ...ToString()) without \n.
  • [SUGGESTION] line 4942: LogError missing trailing \n in ReplayBlocks DisconnectBlock failure
    Same missing-newline issue.
  • [SUGGESTION] line 5126: LogError missing trailing \n in LoadGenesisBlock catch handler
    Same missing-newline issue in the LoadGenesisBlock exception handler.
</details>

Comment thread src/node/blockstorage.cpp
Comment on lines 806 to 810
if (*hash != pindex->GetBlockHash()) {
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Stale return error(...) with unreachable return false; bitcoin#29236 conversion missed this site

Confirmed at src/node/blockstorage.cpp:806-810:

if (*hash != pindex->GetBlockHash()) {
    return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
                 pindex->ToString(), block_pos.ToString());
    return false;
}

Two problems: (1) error(...) already returns false, so the trailing return false; is unreachable — this can trip the configured -Wunreachable-code diagnostics when warnings are promoted to errors. (2) The point of the bitcoin#29236 backport in this PR is precisely to nuke error() from these call sites; every other hunk in the diff converts to LogError(...\n); return false;. This single site was missed during merge resolution.

Functionally the function still returns false on hash mismatch, so consensus is unaffected, but the merge resolution is inconsistent and risks breaking warning-as-error builds.

💡 Suggested change
Suggested change
if (*hash != pindex->GetBlockHash()) {
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
}
if (*hash != pindex->GetBlockHash()) {
LogError("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s\n",
pindex->ToString(), block_pos.ToString());
return false;
}

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/node/blockstorage.cpp`:
- [BLOCKING] lines 806-810: Stale return error(...) with unreachable return false; bitcoin#29236 conversion missed this site
  Confirmed at src/node/blockstorage.cpp:806-810:

```cpp
if (*hash != pindex->GetBlockHash()) {
    return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
                 pindex->ToString(), block_pos.ToString());
    return false;
}

Two problems: (1) error(...) already returns false, so the trailing return false; is unreachable — this can trip the configured -Wunreachable-code diagnostics when warnings are promoted to errors. (2) The point of the bitcoin#29236 backport in this PR is precisely to nuke error() from these call sites; every other hunk in the diff converts to LogError(...\n); return false;. This single site was missed during merge resolution.

Functionally the function still returns false on hash mismatch, so consensus is unaffected, but the merge resolution is inconsistent and risks breaking warning-as-error builds.

</details>

Comment thread src/validation.cpp
CBlock block;
if (!ReadBlockFromDisk(block, pindex, m_params.GetConsensus())) {
return error("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: LogError missing trailing \n in RollforwardBlock

Confirmed: line ends with ...ToString()); and no \n in the format string. LogError expands to LogPrintLevel_LogPrintf_tfm::format(fmt, args...) and writes the result verbatim via LogPrintStr. The Logger uses the trailing \n to drive m_started_new_line (logging.cpp:481), which controls whether the next entry gets its timestamp/source-location/threadname prefix. Without a trailing \n the next log line gets concatenated onto the same line and loses its prefix. Every other LogError introduced by this PR (e.g., lines 4690, 4755, 4897, 4909, 4916) ends in \n; this one was missed.

💡 Suggested change
Suggested change
LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString());

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 4781: LogError missing trailing \n in RollforwardBlock
  Confirmed: line ends with `...ToString());` and no `\n` in the format string. `LogError` expands to `LogPrintLevel_` → `LogPrintf_` → `tfm::format(fmt, args...)` and writes the result verbatim via `LogPrintStr`. The Logger uses the trailing `\n` to drive `m_started_new_line` (logging.cpp:481), which controls whether the next entry gets its timestamp/source-location/threadname prefix. Without a trailing `\n` the next log line gets concatenated onto the same line and loses its prefix. Every other LogError introduced by this PR (e.g., lines 4690, 4755, 4897, 4909, 4916) ends in `\n`; this one was missed.

Comment thread src/validation.cpp
const bool fDIP0003Active = DeploymentActiveAt(*pindexOld, m_params.GetConsensus(), Consensus::DEPLOYMENT_DIP0003);
if (fDIP0003Active && !m_evoDb.VerifyBestBlock(pindexOld->GetBlockHash())) {
return error("ReplayBlocks(DASH): Found EvoDB inconsistency");
LogError("ReplayBlocks(DASH): Found EvoDB inconsistency");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: LogError missing trailing \n on Dash-specific EvoDB inconsistency message

Same root cause as the line 4781 finding: this Dash-specific call (note the (DASH): tag) was converted from error(...) but lost the newline. The very next LogError call at line 4936 (also Dash-specific) has the same defect, as do 4942 and 5126.

💡 Suggested change
Suggested change
LogError("ReplayBlocks(DASH): Found EvoDB inconsistency");
LogError("ReplayBlocks(DASH): Found EvoDB inconsistency\n");

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 4924: LogError missing trailing \n on Dash-specific EvoDB inconsistency message
  Same root cause as the line 4781 finding: this Dash-specific call (note the `(DASH):` tag) was converted from `error(...)` but lost the newline. The very next LogError call at line 4936 (also Dash-specific) has the same defect, as do 4942 and 5126.

Comment thread src/validation.cpp
CBlock block;
if (!ReadBlockFromDisk(block, pindexOld, m_params.GetConsensus())) {
return error("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: LogError missing trailing \n in ReplayBlocks ReadBlockFromDisk failure

Same missing-newline issue as 4781/4924. Format string ends with ...ToString()) without \n.

💡 Suggested change
Suggested change
LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 4936: LogError missing trailing \n in ReplayBlocks ReadBlockFromDisk failure
  Same missing-newline issue as 4781/4924. Format string ends with `...ToString())` without `\n`.

Comment thread src/validation.cpp
DisconnectResult res = DisconnectBlock(block, pindexOld, cache);
if (res == DISCONNECT_FAILED) {
return error("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: LogError missing trailing \n in ReplayBlocks DisconnectBlock failure

Same missing-newline issue.

💡 Suggested change
Suggested change
LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 4942: LogError missing trailing \n in ReplayBlocks DisconnectBlock failure
  Same missing-newline issue.

Comment thread src/validation.cpp
}
} catch (const std::runtime_error &e) {
return error("%s: failed to initialize block database: %s", __func__, e.what());
LogError("%s: failed to initialize block database: %s", __func__, e.what());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: LogError missing trailing \n in LoadGenesisBlock catch handler

Same missing-newline issue in the LoadGenesisBlock exception handler.

💡 Suggested change
Suggested change
LogError("%s: failed to initialize block database: %s", __func__, e.what());
LogError("%s: failed to initialize block database: %s\n", __func__, e.what());

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 5126: LogError missing trailing \n in LoadGenesisBlock catch handler
  Same missing-newline issue in the `LoadGenesisBlock` exception handler.

Comment on lines 87 to +192
@@ -178,6 +180,16 @@ BOOST_AUTO_TEST_CASE(vector_bool)
BOOST_CHECK(SerializeHash(vec1) == SerializeHash(vec2));
}

BOOST_AUTO_TEST_CASE(array)
{
std::array<uint8_t, 32> array1{1, 0, 0, 1, 1, 1, 0, 0, 0, 0, 1, 0, 0, 1, 1, 0, 0, 0, 1, 1, 1, 1, 0, 1, 0, 0, 1};
DataStream ds;
ds << array1;
std::array<uint8_t, 32> array2;
ds >> array2;
BOOST_CHECK(array1 == array2);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: bitcoin#28857 cherry-pick is the test commit only; refactor commit deferred until MessageStartChars→std::array conversion is backported

Verified: bitcoin#28857 upstream had a refactor commit (d49d198) initializing m_magic_bytes via the constructor initializer list using std::array<uint8_t, 4>, and a test commit (1e5b861) adding std::array serialization tests. The Dash cherry-pick (1040ad0) only includes the test piece. src/protocol.h:36 still defines typedef unsigned char MessageStartChars[MESSAGE_START_SIZE]; (a C array), so the upstream constructor-initializer form would not compile here. This is non-blocking — the cherry-pick does not introduce broken code — but the refactor portion of bitcoin#28857 is still pending and will need a follow-up after the upstream MessageStartChars→std::array conversion is backported into Dash.

source: ['claude']

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking

Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Cumulative re-review at 11dde62. One blocking merge-resolution defect at src/node/blockstorage.cpp:806-810 (stale return error(...) with unreachable return false; — the only converted site that still uses the old error() helper, inconsistent with the rest of the bitcoin#29236 conversion and risks tripping warning-as-error). Five LogError sites in src/validation.cpp lost their trailing \n during the conversion (4781, 4924, 4936, 4942, 5126); confirmed against logging.cpp:481 where m_started_new_line depends on the terminator. Two informational notes: bitcoin#28857 cherry-pick is test-only because the MessageStartChars→std::array prerequisite (bitcoin#28423) is not yet backported, and bitcoin#29236 is only partially adopted (Dash retains its error() template at src/util/system.h with several call sites still using it).

Reviewed commit: 11dde62

Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.

Code Review

Reviewed commit: 11dde62

🔴 1 blocking | 🟡 5 suggestion(s) | 💬 2 nitpick(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/node/blockstorage.cpp`:
- [BLOCKING] lines 806-810: Missed bitcoin#29236 conversion: stale `return error(...)` with unreachable `return false;`
  This site was not adapted during the bitcoin#29236 backport. `error(...)` already returns `false`, so the trailing `return false;` is unreachable and can trip `-Wunreachable-code` on warning-as-error builds. It is also the only call site in this PR's diff that still uses the old `error()` helper instead of the new `LogError(...) + return false` pattern, making the merge resolution inconsistent. Functionality is preserved (the function still returns false on hash mismatch), but the inconsistency should be fixed before merge.

In `src/validation.cpp`:
- [SUGGESTION] line 4781: LogError missing trailing `\n` in RollforwardBlock
  `LogError` expands to `LogPrintLevel_` and writes the formatted string verbatim via `LogPrintStr`. The logger uses the trailing `\n` to set `m_started_new_line` (logging.cpp:481), which controls whether the next entry receives its timestamp/source-location/threadname prefix. Without the terminator, the next log line is concatenated onto this one and loses its prefix. Every other LogError introduced by this PR (lines 4690, 4755, 4789, 4860, 4865, 4872, 4879, 4897, 4909, 4916) ends in `\n`; this one was missed during the conversion.
- [SUGGESTION] line 4924: LogError missing trailing `\n` on Dash-specific EvoDB inconsistency log
  Same root cause as line 4781: this Dash-specific call (note the `(DASH):` tag) was converted from `error(...)` but lost its newline. The next log entry will be glued onto this one without its usual prefix.
- [SUGGESTION] line 4936: LogError missing trailing `\n` in ReplayBlocks ReadBlockFromDisk failure
  Same missing-newline regression as 4781/4924. Format string ends with `...ToString())` with no `\n`, so the next log line loses its prefix.
- [SUGGESTION] line 4942: LogError missing trailing `\n` in ReplayBlocks DisconnectBlock failure
  Same missing-newline regression. Without the terminator, subsequent log records are concatenated and lose their normal metadata prefixing — particularly unhelpful when diagnosing replay/recovery failures.
- [SUGGESTION] line 5126: LogError missing trailing `\n` in LoadGenesisBlock catch handler
  Same missing-newline regression in the LoadGenesisBlock exception handler. Startup-time failures here are exactly the cases where preserving the prefix on the next log line matters for diagnosis.

glozow and others added 4 commits May 18, 2026 09:12
bf264e0 test: check_mempool_result negative feerate (kevkevin)

Pull request description:

  Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result`
  Asserts "Amount out of range" error message and `-3` error code

  Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250)

ACKs for top commit:
  maflcko:
    lgtm ACK bf264e0
  brunoerg:
    nice, utACK bf264e0
  davidgumberg:
    Looks great, ACK bitcoin@bf264e0

Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78
80fa7da test: Refactor subtree exclusion in lint tests (Brandon Odiwuor)

Pull request description:

  Fixes bitcoin#17413

  Refactor subtree exclusion in lint tests to one place

  Second attempt after PR: bitcoin#24435

ACKs for top commit:
  fjahr:
    re-ACK 80fa7da
  maflcko:
    lgtm ACK 80fa7da
  davidgumberg:
    ACK bitcoin@80fa7da

Tree-SHA512: deff7457dd19ca5ea440d3d53feae047e8863b9ddeb6494a3c94605a5d16edc91db8f99a435b4fab2ef89aedee42439562be006da647fb85bbf3def903a3ce50
… add unit test

3e9c736 test: fix accurate multisig sigop count (BIP16), add unit test (Sebastian Falbesoner)

Pull request description:

  In the course of reviewing bitcoin#29589 I noticed the following buggy call-site of `CScriptOp.decode_op_n` in the CScript's `GetSigOpCount` method:
  https://github.com/bitcoin/bitcoin/blob/4cc99df44aec4d104590aee46cf18318e22a8568/test/functional/test_framework/script.py#L591-L593
  This should be `lastOpcode` rather than `opcode`. The latter is either OP_CHECKMULTISIG or OP_CHECKMULTISIGVERIFY at this point, so `decode_op_n` would result in an error. Also, in `CScript.raw_iter`, we have to return the op as `CScriptOp` type instead of a bare integer, otherwise we can't call the decode method on it. To prevent this in the future, add some simple unit tests for `GetSigOpCount`.

  Note that this was unnoticed, as the code part was never hit so far in the test framework.

ACKs for top commit:
  achow101:
    ACK 3e9c736
  Christewart:
    ACK 3e9c736
  rkrux:
    tACK [3e9c736](bitcoin@3e9c736)
  hernanmarino:
    tACK 3e9c736

Tree-SHA512: 51647bb6d462fbd101effd851afdbd6ad198c0567888cd4fdcac389a9fb4bd3d7e648095c6944fd8875d36272107ebaabdc62d0e2423289055588c12294d05a7
…output text in json format

9adf949 contrib: rpcauth.py - Add new option (-j/--json) to output text in json format (bstin)

Pull request description:

  This is a simple change to rpcauth.py utility in order to output as json instead raw text.

  This is beneficial because integrating json output is simpler with multiple different forms of automation and tooling

ACKs for top commit:
  maflcko:
    ACK 9adf949
  achow101:
    ACK 9adf949
  willcl-ark:
    tACK 9adf949
  tdb3:
    ACK for 9adf949

Tree-SHA512: 2cdc3b2071fbe4fb32a84ce42ee8ad216cff96ed82aaef58daeb3991953ac137ae42d6898a7fdb6cbd1800e1f61ff8d292f0b150eaebdd2a3fd9d37ed7450787
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin/bitcoin#28857 , 29458, 29236, 29633, 29459, 29479, 29615, 29433 backport: Merge bitcoin/bitcoin#28857 , 29458, 29236, 29459, 29479, 29615, 29433 May 18, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
test/lint/lint-spelling.py (1)

18-18: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rename loop variable dir to avoid shadowing Python built-in.

Line 18 uses dir as a loop variable, which shadows the built-in dir() function and triggers Ruff A001. This will fail lint CI checks.

🔧 Proposed fix
-FILES_ARGS += [f":(exclude){dir}" for dir in SHARED_EXCLUDED_SUBTREES]
+FILES_ARGS += [f":(exclude){subtree}" for subtree in SHARED_EXCLUDED_SUBTREES]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/lint/lint-spelling.py` at line 18, The comprehension uses the loop
variable named dir which shadows the built-in dir() and causes lint A001; rename
that variable (for example to subdir or excl) in the expression that builds
FILES_ARGS so the line reads FILES_ARGS += [f":(exclude){subdir}" for subdir in
SHARED_EXCLUDED_SUBTREES], updating any related references to use the new name
and leaving FILES_ARGS and SHARED_EXCLUDED_SUBTREES unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/lint/lint_ignore_dirs.py`:
- Around line 1-5: The list assignment for SHARED_EXCLUDED_SUBTREES has
misaligned continuation lines causing Flake8 E128/E124; fix it by consistently
aligning the items and the closing bracket (either put all items on the same
line or place each string on its own line indented to the same column under the
opening bracket), e.g. align each entry under the first list element and ensure
the closing ] lines up with the start of the assignment so
SHARED_EXCLUDED_SUBTREES is properly formatted.

In `@test/lint/lint-includes.py`:
- Around line 20-26: EXCLUDED_DIRS currently hardcodes directories that are also
present in SHARED_EXCLUDED_SUBTREES causing duplicates; update the EXCLUDED_DIRS
definition to avoid repeating entries by either removing the overlapping
hardcoded entries (leave only the unique ones plus SHARED_EXCLUDED_SUBTREES) or
build EXCLUDED_DIRS by concatenating and then deduplicating while preserving
order (e.g., use an ordered set/dedup logic) so that EXCLUDED_DIRS and
SHARED_EXCLUDED_SUBTREES do not contain duplicated paths; target the
EXCLUDED_DIRS symbol in the diff to implement the change.

---

Duplicate comments:
In `@test/lint/lint-spelling.py`:
- Line 18: The comprehension uses the loop variable named dir which shadows the
built-in dir() and causes lint A001; rename that variable (for example to subdir
or excl) in the expression that builds FILES_ARGS so the line reads FILES_ARGS
+= [f":(exclude){subdir}" for subdir in SHARED_EXCLUDED_SUBTREES], updating any
related references to use the new name and leaving FILES_ARGS and
SHARED_EXCLUDED_SUBTREES unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 977db31c-4dd7-4bd3-808c-c787f898b7a9

📥 Commits

Reviewing files that changed from the base of the PR and between 11dde62 and 16c7f15.

📒 Files selected for processing (9)
  • share/rpcauth/README.md
  • share/rpcauth/rpcauth.py
  • test/functional/mempool_accept.py
  • test/functional/test_framework/script.py
  • test/lint/README.md
  • test/lint/lint-include-guards.py
  • test/lint/lint-includes.py
  • test/lint/lint-spelling.py
  • test/lint/lint_ignore_dirs.py
✅ Files skipped from review due to trivial changes (2)
  • share/rpcauth/README.md
  • test/lint/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • share/rpcauth/rpcauth.py
  • test/functional/mempool_accept.py
  • test/functional/test_framework/script.py

Comment on lines +1 to +5
SHARED_EXCLUDED_SUBTREES = ["src/leveldb/",
"src/crc32c/",
"src/secp256k1/",
"src/minisketch/",
]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix list indentation to satisfy Flake8 checks.

The continuation lines are not properly aligned, triggering E128 and E124 violations that will fail CI lint checks.

📐 Proposed fix
-SHARED_EXCLUDED_SUBTREES = ["src/leveldb/",
-                 "src/crc32c/",
-                 "src/secp256k1/",
-                 "src/minisketch/",
-                ]
+SHARED_EXCLUDED_SUBTREES = [
+    "src/leveldb/",
+    "src/crc32c/",
+    "src/secp256k1/",
+    "src/minisketch/",
+]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
SHARED_EXCLUDED_SUBTREES = ["src/leveldb/",
"src/crc32c/",
"src/secp256k1/",
"src/minisketch/",
]
SHARED_EXCLUDED_SUBTREES = [
"src/leveldb/",
"src/crc32c/",
"src/secp256k1/",
"src/minisketch/",
]
🧰 Tools
🪛 Flake8 (7.3.0)

[error] 2-2: continuation line under-indented for visual indent

(E128)


[error] 3-3: continuation line under-indented for visual indent

(E128)


[error] 4-4: continuation line under-indented for visual indent

(E128)


[error] 5-5: closing bracket does not match visual indentation

(E124)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/lint/lint_ignore_dirs.py` around lines 1 - 5, The list assignment for
SHARED_EXCLUDED_SUBTREES has misaligned continuation lines causing Flake8
E128/E124; fix it by consistently aligning the items and the closing bracket
(either put all items on the same line or place each string on its own line
indented to the same column under the opening bracket), e.g. align each entry
under the first list element and ensure the closing ] lines up with the start of
the assignment so SHARED_EXCLUDED_SUBTREES is properly formatted.

Comment on lines 20 to +26
EXCLUDED_DIRS = ["src/leveldb/",
"src/crc32c/",
"src/secp256k1/",
"src/minisketch/",
"src/dashbls/",
"src/immer/",
"src/crypto/x11/"]
"src/crypto/x11/"] + SHARED_EXCLUDED_SUBTREES
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove duplicate directory entries from EXCLUDED_DIRS.

Lines 20-23 hardcode src/leveldb/, src/crc32c/, src/secp256k1/, and src/minisketch/, but line 26 appends SHARED_EXCLUDED_SUBTREES which contains the same four directories. This creates duplicate entries and defeats the purpose of centralizing the exclusion list.

🔧 Proposed fix
-EXCLUDED_DIRS = ["src/leveldb/",
-                 "src/crc32c/",
-                 "src/secp256k1/",
-                 "src/minisketch/",
-                 "src/dashbls/",
+EXCLUDED_DIRS = ["src/dashbls/",
                  "src/immer/",
                  "src/crypto/x11/"] + SHARED_EXCLUDED_SUBTREES
🧰 Tools
🪛 Ruff (0.15.12)

[warning] 20-26: Consider iterable unpacking instead of concatenation

Replace with iterable unpacking

(RUF005)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/lint/lint-includes.py` around lines 20 - 26, EXCLUDED_DIRS currently
hardcodes directories that are also present in SHARED_EXCLUDED_SUBTREES causing
duplicates; update the EXCLUDED_DIRS definition to avoid repeating entries by
either removing the overlapping hardcoded entries (leave only the unique ones
plus SHARED_EXCLUDED_SUBTREES) or build EXCLUDED_DIRS by concatenating and then
deduplicating while preserving order (e.g., use an ordered set/dedup logic) so
that EXCLUDED_DIRS and SHARED_EXCLUDED_SUBTREES do not contain duplicated paths;
target the EXCLUDED_DIRS symbol in the diff to implement the change.

Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

FAILED TO PARSE: no review JSON object found

Reviewed commit: 16c7f15

Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

On 16c7f15bcd22f9fa1a71b2d64499b461078734e7, the concrete issues are limited to logging regressions introduced by the error(...) to LogError(...) conversion and one leftover dead-code artifact in ReadBlockFromDisk. The broader backport-prerequisite notes about bitcoin#28857 and bitcoin#29236 are accurate as repository history observations, but they are not actionable defects in this checked-out PR state and should not be posted as review findings.

Correction: this supersedes my immediately previous parser-error review body for this same run; the verified findings below are the intended review output.

Reviewed commit: 16c7f15

🟡 3 suggestion(s) | 💬 1 nitpick(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 4781: `RollforwardBlock` leaves the log line unterminated after the `LogError` conversion
  `LogError` does not append a newline on its own. In `Logger::LogPrintStr`, `m_started_new_line` is only reset when the format string ends with `\n`, so this message leaves the logger in the middle of a line and the next record is emitted without its normal timestamp/category prefix. That makes block-replay failures materially harder to diagnose.
- [SUGGESTION] lines 4924-4942: `ReplayBlocks` has three unterminated `LogError` messages
  All three `LogError` calls added in this `ReplayBlocks` path omit a trailing newline. Because the logger prefixes only the start of a new line, any later log entry after one of these failures will be concatenated onto the same physical line and lose its normal metadata prefix. This affects EvoDB inconsistency reporting, rollback disk-read failures, and disconnect failures.
- [SUGGESTION] line 5126: `LoadGenesisBlock` logs startup failure without terminating the record
  The `catch` block was converted from `error(...)` to `LogError(...)`, but the newline was dropped. If genesis-block initialization throws, the next log record is appended to the same line and loses its usual prefix, which degrades exactly the startup diagnostics this path is supposed to preserve.

Comment thread src/validation.cpp
CBlock block;
if (!ReadBlockFromDisk(block, pindex, m_params.GetConsensus())) {
return error("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: RollforwardBlock leaves the log line unterminated after the LogError conversion

LogError does not append a newline on its own. In Logger::LogPrintStr, m_started_new_line is only reset when the format string ends with \n, so this message leaves the logger in the middle of a line and the next record is emitted without its normal timestamp/category prefix. That makes block-replay failures materially harder to diagnose.

💡 Suggested change
Suggested change
LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
LogError("RollforwardBlock(): ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString());

source: ['codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 4781: `RollforwardBlock` leaves the log line unterminated after the `LogError` conversion
  `LogError` does not append a newline on its own. In `Logger::LogPrintStr`, `m_started_new_line` is only reset when the format string ends with `\n`, so this message leaves the logger in the middle of a line and the next record is emitted without its normal timestamp/category prefix. That makes block-replay failures materially harder to diagnose.

Comment thread src/validation.cpp
Comment on lines 4924 to +4942
@@ -4892,12 +4933,14 @@ bool CChainState::ReplayBlocks()
if (pindexOld->nHeight > 0) { // Never disconnect the genesis block.
CBlock block;
if (!ReadBlockFromDisk(block, pindexOld, m_params.GetConsensus())) {
return error("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
return false;
}
LogPrintf("Rolling back %s (%i)\n", pindexOld->GetBlockHash().ToString(), pindexOld->nHeight);
DisconnectResult res = DisconnectBlock(block, pindexOld, cache);
if (res == DISCONNECT_FAILED) {
return error("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: ReplayBlocks has three unterminated LogError messages

All three LogError calls added in this ReplayBlocks path omit a trailing newline. Because the logger prefixes only the start of a new line, any later log entry after one of these failures will be concatenated onto the same physical line and lose its normal metadata prefix. This affects EvoDB inconsistency reporting, rollback disk-read failures, and disconnect failures.

💡 Suggested change
Suggested change
if (fDIP0003Active && !m_evoDb.VerifyBestBlock(pindexOld->GetBlockHash())) {
LogError("ReplayBlocks(DASH): Found EvoDB inconsistency\n");
return false;
}
}
auto dbTx = m_evoDb.BeginTransaction();
// Rollback along the old branch.
while (pindexOld != pindexFork) {
if (pindexOld->nHeight > 0) { // Never disconnect the genesis block.
CBlock block;
if (!ReadBlockFromDisk(block, pindexOld, m_params.GetConsensus())) {
LogError("ReplayBlocks(): ReadBlockFromDisk() failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
return false;
}
LogPrintf("Rolling back %s (%i)\n", pindexOld->GetBlockHash().ToString(), pindexOld->nHeight);
DisconnectResult res = DisconnectBlock(block, pindexOld, cache);
if (res == DISCONNECT_FAILED) {
LogError("ReplayBlocks(): DisconnectBlock failed at %d, hash=%s\n", pindexOld->nHeight, pindexOld->GetBlockHash().ToString());
return false;
}

source: ['codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] lines 4924-4942: `ReplayBlocks` has three unterminated `LogError` messages
  All three `LogError` calls added in this `ReplayBlocks` path omit a trailing newline. Because the logger prefixes only the start of a new line, any later log entry after one of these failures will be concatenated onto the same physical line and lose its normal metadata prefix. This affects EvoDB inconsistency reporting, rollback disk-read failures, and disconnect failures.

Comment thread src/validation.cpp
}
} catch (const std::runtime_error &e) {
return error("%s: failed to initialize block database: %s", __func__, e.what());
LogError("%s: failed to initialize block database: %s", __func__, e.what());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: LoadGenesisBlock logs startup failure without terminating the record

The catch block was converted from error(...) to LogError(...), but the newline was dropped. If genesis-block initialization throws, the next log record is appended to the same line and loses its usual prefix, which degrades exactly the startup diagnostics this path is supposed to preserve.

💡 Suggested change
Suggested change
LogError("%s: failed to initialize block database: %s", __func__, e.what());
LogError("%s: failed to initialize block database: %s\n", __func__, e.what());

source: ['codex']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] line 5126: `LoadGenesisBlock` logs startup failure without terminating the record
  The `catch` block was converted from `error(...)` to `LogError(...)`, but the newline was dropped. If genesis-block initialization throws, the next log record is appended to the same line and loses its usual prefix, which degrades exactly the startup diagnostics this path is supposed to preserve.

Comment thread src/node/blockstorage.cpp
Comment on lines 806 to +809
if (*hash != pindex->GetBlockHash()) {
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: ReadBlockFromDisk still contains a dead return false; after return error(...)

This branch still uses the old error(...) helper, which already logs and returns false. The following return false; is unreachable dead code. It does not change runtime behavior, but it is a leftover from the partial bitcoin#29236 conversion in a function touched by this PR and should be cleaned up rather than carried forward.

💡 Suggested change
Suggested change
if (*hash != pindex->GetBlockHash()) {
return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
pindex->ToString(), block_pos.ToString());
return false;
LogError("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s\n",
pindex->ToString(), block_pos.ToString());
return false;

source: ['codex']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants